Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension is slow to activate #4226 #4256

Merged

Conversation

vrubezhny
Copy link
Contributor

@vrubezhny vrubezhny commented Jul 3, 2024

The PR adds some optimization to the extension activation.

Also, the PR removes function 'migrateFromOdo018' which looks over outdated and doesn't make its work properly after a numerous changes in ODO. See: #4255

Fixes: #4255
Fixes: #4226

@vrubezhny
Copy link
Contributor Author

My calculations for the extension loading time before (installed from MS Marketplace) and after the fix (installed from locally built VSIX) differ in numbers, here are some examples:

  • Before the fix (installed from MS Marketplace):
    Screenshot from 2024-07-03 22-45-44

  • After the fix (installed from locally built VSIX):
    Screenshot from 2024-07-03 22-40-32

@vrubezhny vrubezhny requested a review from datho7561 July 3, 2024 20:59
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 43.92%. Comparing base (da60441) to head (50b346f).
Report is 339 commits behind head on main.

Files Patch % Lines
src/extension.ts 95.83% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4256       +/-   ##
===========================================
+ Coverage   32.37%   43.92%   +11.55%     
===========================================
  Files          85       95       +10     
  Lines        6505     7699     +1194     
  Branches     1349     1647      +298     
===========================================
+ Hits         2106     3382     +1276     
+ Misses       4399     4317       -82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty good so far. Noticeably speeds up the extension start up. I think the main speed up is not blocking on registering the commands/webviews on startup. I was worried this would introduce concurrency bugs, but it doesn't seem to introduce any.

I've added two small comments.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@vrubezhny
Copy link
Contributor Author

vrubezhny commented Jul 4, 2024

There's still something strange in the extension activation time calculation...

image

The extension's activate method finishes in ~66ms, while the VSCode Running Extensions view says it's activated in ~1186ms... I understand it may include the time needed to load Red Hat Authentication extension we depend on, but its reported activation time is only ~74ms in my case... How does it calculate 1186 ms?.. IDK

@vrubezhny vrubezhny force-pushed the fix-optimize-extension-activation branch from b336974 to a995b66 Compare July 4, 2024 22:10
@vrubezhny
Copy link
Contributor Author

vrubezhny commented Jul 4, 2024

Seems pretty good so far. Noticeably speeds up the extension start up. I think the main speed up is not blocking on registering the commands/webviews on startup. I was worried this would introduce concurrency bugs, but it doesn't seem to introduce any.

Still trying to introduce anything that breaks it 😎 ... I've turned setting up the Kube Context call to be async (it takes quite a lot of time to finish and also it might require a user intervention, so...) and wait at the end for its promise to be resolved... Still doesn't look like I broke anything (even the status bar active context and project items set up).

@datho7561 Anyway, take a look please

I've added two small comments.

Done

@vrubezhny vrubezhny requested a review from datho7561 July 4, 2024 22:29
@vrubezhny vrubezhny force-pushed the fix-optimize-extension-activation branch from a995b66 to 8452887 Compare July 4, 2024 22:45
The PR adds some optimization to the extension activation.

Also, the PR removes function 'migrateFromOdo018' which looks over outdated and doesn't make its
work properly after a numerous changes in ODO. See: redhat-developer#4255

Fixes: redhat-developer#4255
Fixes: redhat-developer#4226

Signed-off-by: Victor Rubezhny <[email protected]>
@vrubezhny vrubezhny force-pushed the fix-optimize-extension-activation branch from 8452887 to 50b346f Compare July 4, 2024 23:19
@datho7561
Copy link
Contributor

datho7561 commented Jul 5, 2024

Try out this patch to do the timing:

diff --git a/src/extension.ts b/src/extension.ts
index 62cdd465..4a39e36d 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -3,6 +3,8 @@
  *  Licensed under the MIT License. See LICENSE file in the project root for license information.
  *-----------------------------------------------------------------------------------------------*/
 
+const startTime = new Date().getTime();
+import { Context as KcuContext } from '@kubernetes/client-node/dist/config_types';
 import {
     authentication,
     commands, env, ExtensionContext, languages, QuickPickItemKind,
@@ -25,16 +27,15 @@ import { startTelemetry } from './telemetry';
 import { ToolsConfig } from './tools';
 import { TokenStore } from './util/credentialManager';
 import { getNamespaceKind, KubeConfigUtils, setKubeConfig } from './util/kubeUtils';
-import { Context as KcuContext } from '@kubernetes/client-node/dist/config_types';
 import { setupWorkspaceDevfileContext } from './util/workspace';
 import { registerCommands } from './vscommand';
 import { OpenShiftTerminalManager } from './webview/openshift-terminal/openShiftTerminal';
 import { WelcomePage } from './welcomePage';
 import { registerYamlHandlers } from './yaml/yamlDocumentFeatures';
 
-import { Oc } from './oc/ocWrapper';
-import { K8S_RESOURCE_SCHEME, K8S_RESOURCE_SCHEME_READONLY, KubernetesResourceVirtualFileSystemProvider } from './k8s/vfs/kuberesources.virtualfs';
 import { KubernetesResourceLinkProvider } from './k8s/vfs/kuberesources.linkprovider';
+import { K8S_RESOURCE_SCHEME, K8S_RESOURCE_SCHEME_READONLY, KubernetesResourceVirtualFileSystemProvider } from './k8s/vfs/kuberesources.virtualfs';
+import { Oc } from './oc/ocWrapper';
 
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 // this method is called when your extension is deactivated
@@ -276,6 +277,9 @@ export async function activate(extensionContext: ExtensionContext): Promise<unkn
     // Wait for finishing Kube Config setup
     await setKubeConfigPromise;
 
+    // eslint-disable-next-line no-console
+    console.log(`vscode-openshift activation time: ${new Date().getTime() - startTime}`);
+
     return {
         verifyBundledBinaries
     };

@vrubezhny
Copy link
Contributor Author

vrubezhny commented Jul 5, 2024

Try out this patch to do the timing:

Yep...
image

Now the reported activation times are closer to each other... even the vscode's time is less than the Extension activation time (?)...

But I cannot do anything to speed up the time spent to import the required dependencies... Can I?

We might need to revisit the list of disposables - if we can exclude anything, it could reduce the list of imports as well.

UPD: I couldn't find anything to remove from the list of disposables.

@datho7561
Copy link
Contributor

But I cannot do anything to speed up the time spent to import the required dependencies... Can I?

Not really. Maybe we could all the code into one file, but we would need to make sure that we don't use any libraries that prevent that, and we can investigate/do that in a future PR.

I'll take another look at the changes and make sure it looks good.

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the trick for me. I no longer have to wait a few seconds to interact with the Application Explorer. It loads in what feels like around a second.

I don't know if you want to wait on feedback from Fred, or go ahead and merge. Either is fine to me.

@vrubezhny
Copy link
Contributor Author

Maybe we could all the code into one file, but we would need to make sure that we don't use any libraries that prevent that, and we can investigate/do that in a future PR.

Yeah. I got the idea.

@datho7561 Let's merge this one. And then we can ask Fred for his feedback and improve it later.

@vrubezhny vrubezhny merged commit 86b746c into redhat-developer:main Jul 5, 2024
4 checks passed
@rgrunber rgrunber mentioned this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension is slow to activate Remove outdated 'migrateFromOdo018' method
3 participants